Recognize the MS-DIAL 5 suggested-annotation name prefixes - #784
Merged
YukiMatsuzawa merged 2 commits intoSep 4, 2026
Conversation
MS-DIAL 4 marked its precursor-only suggestions with a single "w/o MS2:" prefix. MS-DIAL 5 splits that bucket in two: SetMoleculeMsPropertyAsSuggested writes "no MS2: " when the feature has no product-ion spectrum (MS2RawSpectrumID < 0) and "low score: " when a product-ion spectrum exists but the reference search did not meet the acceptance criteria. Two readers still tested only the MS-DIAL 4 spelling, which never matches MS-DIAL 5 output, so both suggestion shapes were treated as accepted annotations: - MoleculerNetworking.GetOntologyColor gave precursor-only and low-score suggestions an ontology colour on the molecular network, identical to a confident annotation, on a graph whose edges are MS/MS similarity. - DataAccess.IsReferenceMatchedName reported them as Reference matched in the Console quality-assurance matrix. Both name shapes are produced only on the branches where MsScanMatchResult.IsReferenceMatched is false, so neither is an accepted reference match. "w/o MS2: " is kept: SetPeptideMsPropertyAsSuggested still writes it. The prefix vocabulary and the predicate now live together in CompMs.Common.Utility.AnnotationName, so a renamed prefix cannot leave a reader stale, and the writers build their names from it. The helper is in CommonStandard because the vocabulary is shared by CommonStandard and MsdialCore and the project dependency runs MsdialCore -> CommonStandard. Verified on the console_fastlc_demo LC-MS run (7 SCIEX WIFF files): mdalign and mzTab-M output are byte-identical, and the quality-assurance matrix changes in one direction only, 3919 rows TRUE -> FALSE and none the other way. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
Newly added test files are missing required using directives, which will prevent the test projects from compiling.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
This PR centralizes MS-DIAL “suggested annotation” name prefixes and the “is this an accepted reference match?” predicate so that both MS-DIAL 5 suggestion shapes (no MS2: / low score:) are consistently excluded by downstream readers (molecular networking node coloring and Console QA export).
Changes:
- Added
CompMs.Common.Utility.AnnotationNameto own suggestion-prefix vocabulary and theIsReferenceMatched()predicate. - Updated MS-DIAL 5 name writers (
DataAccess.Set*M*AsSuggested) and readers (DataAccess.IsReferenceMatchedName,MoleculerNetworking.GetOntologyColor) to useAnnotationName. - Added regression tests to cover MS-DIAL 5 suggestion prefixes and the writer/reader invariant.
File summaries
| File | Description |
|---|---|
| tests/MSDIAL5/MsdialCoreTests/Utility/DataAccessTests.cs | Extends QA/export test cases to include MS-DIAL 5 suggestion prefixes. |
| tests/Common/CommonStandardTests/Utility/AnnotationNameTests.cs | New unit tests for the centralized suggestion-prefix predicate and writers. |
| tests/Common/CommonStandardTests/Algorithm/Function/MoleculerNetworkingTests.cs | New regression tests ensuring suggestion-shaped names do not receive ontology coloring. |
| src/MSDIAL5/MsdialCore/Utility/DataAccess.cs | Uses AnnotationName for suggestion naming and delegates IsReferenceMatchedName() to it. |
| src/Common/CommonStandard/Utility/AnnotationName.cs | New shared helper defining prefixes and the IsReferenceMatched() predicate. |
| src/Common/CommonStandard/Algorithm/Function/MoleculerNetworking.cs | Replaces ad-hoc Contains(...) checks with AnnotationName.IsReferenceMatched(...). |
Review details
- Files reviewed: 6/6 changed files
- Comments generated: 2
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| @@ -0,0 +1,78 @@ | |||
| using CompMs.Common.Components; | |||
Comment on lines
+1
to
+2
| using Microsoft.VisualStudio.TestTools.UnitTesting; | ||
|
|
…ed-annotation-name-predicate
YukiMatsuzawa
deleted the
fix/msdial5-suggested-annotation-name-predicate
branch
September 4, 2026 07:05
This was referenced Sep 4, 2026
htsugawa
added a commit
that referenced
this pull request
Sep 5, 2026
* Export alignment spectrum provenance links * Add compact alignment peak ID matrix export * Stop the provenance sidecar asserting what the run did not establish Review of the rebased branch found the audit sidecar reporting three things that were not measurements, all of them invisible to the existing tests because both tests asserted substrings of a row that had a real source peak, and stopped before the mz column. The mz column was read from ChromXsTop.Mz in the in-memory overload and from Mass in the light overload. On an aligned peak ChromXsTop carries the chromatogram axis, and the gap filler resets it outright, so the column reported a sentinel for every member that DID have a source peak and a real value only for the gap-filled ones. Measured on the FastLC demo: 7879 of 7879 rows with has_source_peak=true carried mz=-1, and 9992 of 9992 gap-filled rows carried a plausible m/z. Exactly backwards, in every row. Both overloads now read Mass, which is also what the .mdalign MZ column uses. A member with no source peak has no source spectrum either, but the gap filler leaves the spectrum ids at their default 0 while resetting only the peak ids, so every gap-filled row published ms1_raw_spectrum_id=0, ms1_raw_spectrum_id_top=0 and ms2_raw_spectrum_id=0. A 0 is a real scan index. In a file whose whole purpose is provenance, that points an auditor at a spectrum belonging to some other peak. All five raw-spectrum columns are now withheld when there is no source peak, in both overloads; the light overload had additionally gated ms2_raw_spectrum_ids on MS2RawSpectrumID >= 0, which is true for that same 0. The peak-id columns published -2 for a gap-filled cell while the compact peak-ID matrix published -1 for the same cell, so two files shipped by one feature disagreed numerically. They now agree on -1, and the distinction the raw values carried is preserved by name in a new peak_origin column, valued detected, gap_filled or absent. That is the same lesson as PR #785: name the state rather than encode it in a magic number, so a reader does not need the gap filler's internals to interpret a cell. Also fix the light-mode QA matrix's "Reference matched" column. The switch to the file-backed quant accessor made light mode derive it from RepresentativeLibraryID >= 0, but a suggested annotation carries a library id too, so a precursor-only or low-score suggestion counted as a reference match in light mode while normal mode reported false. Normal mode resolves the column through AnnotationName.IsReferenceMatched, the predicate PR #784 centralised. The store now persists the representative's own IsReferenceMatched verdict and the accessor reads that. RecordSize goes 102 to 103; the file is a per-run temporary spill created by Path.GetTempFileName, so the layout carries no cross-version obligation. Two guards so this class of defect cannot return silently. WriteMember now goes through a WriteRow that refuses a field count the header does not declare, since the header and the two value lists are three hand-maintained arrays. And the tests assert whole lines rather than substrings, and cover a gap-filled row and an unwritten light row in both overloads, which is the case that hid all three divergences. Build: Release net48, 0 errors. Tests: MsdialCoreTests 316/316 (311 before, plus five new), MsdialCoreTestAppTests 13/13 including a new ReadDetailedAlignmentProvenance test alongside master's ReadLbmAnnotatorPriority test in the project that actually compiles the rebased ConfigParser. Verified end to end on the FastLC demo, ingesting the output into the companion msdial_spectrum_catalog: every member with a source peak now carries a usable m/z, no member without one carries a scan index, and peak_origin splits the 17871 members into 7879 detected and 9992 gap_filled. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> --------- Co-authored-by: DESKTOP-382ETUR\Hiroshi Tsugawa <htsugawa@go.tuat.ac.jp> Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
MS-DIAL 4 marked its precursor-only suggestions with a single
"w/o MS2:"prefix. MS-DIAL 5 splits that bucket in two, inDataAccess.SetMoleculeMsPropertyAsSuggested:no MS2: XMS2RawSpectrumID < 0— no product-ion spectrum at alllow score: XThe
"w/o MS2: "line inSetMoleculeMsPropertyis commented out. Two readers still tested only the MS-DIAL 4 spelling, which never matches MS-DIAL 5 output:MoleculerNetworking.GetOntologyColorgave precursor-only and low-score suggestions an ontology colour on the molecular network, identical to a confident annotation — on a graph whose edges are built from MS/MS similarity, so a miscoloured node seeds false annotation propagation by eye. Reachable fromMsdialGuiApponly; the Consolemsncommand uses the edge-only statics and does not build nodes.DataAccess.IsReferenceMatchedName(added in Feature/console retention time correction #766) reported them asReference matchedin the Console quality-assurance matrix. This is the Console path MS-DIAL Interactive drives.Measured on the checked-out
console_fastlc_demooutput: the.mdalignholds 1021 rows startingno MS2:, 230 startinglow score:, and zero startingw/o MS2.Decision
Both shapes are excluded. They are produced only on the branches where
MsScanMatchResult.IsReferenceMatchedis false (MsScanMatchingsets it asIsSpectrumMatch && TotalScore > cutoff && RT/RI match), so neither is an accepted reference match. A low-score row does have product-ion evidence and ranks above a precursor-only one, but the ontology colour and theReference matchedflag are both binary claims about an accepted annotation, and neither shape qualifies. Preserving the low-score/precursor-only distinction visually would be a third state — separate work, not this predicate."w/o MS2: "is kept:SetPeptideMsPropertyAsSuggestedstill writes it, and MS-DIAL 4 wrote it everywhere. The MS-DIAL 4 sources undersrc/MSDIAL4/are correct as they stand and are untouched.Change
The prefix vocabulary and the predicate now live together in
CompMs.Common.Utility.AnnotationName, and the writers build their names from it, so a renamed prefix cannot leave a reader stale. The helper is in CommonStandard because the vocabulary is shared by CommonStandard (networking) and MsdialCore (export) and the project dependency runs MsdialCore → CommonStandard only.DataAccess.IsReferenceMatchedNameis kept as a delegating overload.One behavioural refinement beyond the added prefixes:
GetOntologyColormoves fromContainsto the helper'sStartsWith-after-TrimStartmatching, so a legitimate compound name that merely containsUnknownorRIKENmid-string is no longer misread. All MS-DIAL 5 shapes are prefixes.IsReferenceMatchedNamealready usedStartsWith, so for the Console path the change is purely additive.Verification
CommonStandardTests844/844 pass;MsdialCoreTests295/295;MsdialLcMsApiTests66 pass / 2 skipped.Expected:<rgb(0,0,0)>. Actual:<rgb(255, 165, 0)>, reproducing the reported symptom.MSDIALCUIbuilt Release/net48 (5.5.241113) and theconsole_fastlc_demoLC-MS run re-executed over its 7 SCIEX WIFF files, exit 0..mdalignbyte-identical to the pre-fix run..mzTabidentical once the job id is normalized. In.qa.tsv, columns 1–11 are byte-identical and column 12 changes in one direction only: 3919 rows TRUE → FALSE, 0 FALSE → TRUE, with the 1196 genuine TRUE rows preserved.Follow-up, deliberately not in this PR
MztabFormatExport.cs(lines 172, 309, 495) testsContains("no MS2")and so already excludes precursor-only suggestions, but notlow score. Whether low-score rows belong in mzTab-M is a separate scientific question about that export, so the exporter is untouched here — including its string literals.MoleculerSpectrumNetworkingTest.MergeNodeFilesin the Console test app carries the same stale literal, but its only caller is commented out, so it has no behavioural effect.🤖 Generated with Claude Code